Skip to content

-r,-a,-l のオプションに対応やり直し#8

Open
Yatsu0720 wants to merge 1 commit intomainfrom
larl
Open

-r,-a,-l のオプションに対応やり直し#8
Yatsu0720 wants to merge 1 commit intomainfrom
larl

Conversation

@Yatsu0720
Copy link
Owner

lコマンドの -r,-a,-l のオプションに対応しましたので、確認お願いいたします。

Copy link

@JunichiIto JunichiIto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コメントしました!

@@ -1,4 +1,3 @@
#!/usr/bin/env ruby

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

質問)この行、なぜ消したんでしょう?

print_l_option_files(file_stats)
else
print_no_option_files(file_stats)
print_not_l_option_files(file_stats)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

man ls-lの説明を読んでみてください。
たぶんlはlong formatのlだと説明されているはずです。
なので、メソッド名もprint_in_long_format、print_in_short_formatみたいにすると対称性があって良いかもしれません。(好みの問題です)


def make_file_stats(ls_dir)
files = fetch_files(ls_dir)
def make_file_stats(ls_dir, params)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「オブジェクトを作る」の「作る」はmakeよりもbuildが使われることが多いので、ここもbuild_xxxにした方がいいかも

files = if params['a']
Dir.glob("#{ls_dir}/*", File::FNM_DOTMATCH).sort
else
Dir.glob("#{ls_dir}/*").sort

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flagのデフォルト値は数値の0のようです。
https://docs.ruby-lang.org/ja/latest/method/Dir/s/=5b=5d.html

なので、

flags = params['a'] ? File::FNM_DOTMATCH : 0

のようにすると、ここのコードがよりDRYにできそうです

あと、前回紹介したFile.joinを使ってパスを連結してみてください〜。

if params['r']
files.reverse
else
files

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

三項演算子を使ってもいいかもしれませんね

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants